Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rac2: return tracked deductions on close/removal #130684

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

kvoli
Copy link
Collaborator

@kvoli kvoli commented Sep 13, 2024

When a replica was removed from a range, or the range controller closed, any tracked deductions did not return tokens to the respective stream token counts.

Update CloseRaftMuLocked and SetReplicasRaftMuLocked to close any replica send streams for replicas which are no longer part of the replica set, closing all when the range controller closes.

Also immediately close the send stream when a replica transitions to StateSnapshot.

Resolves: #130683
Release note: None

@kvoli kvoli self-assigned this Sep 13, 2024
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@kvoli kvoli force-pushed the 240913.rac-send-stream-close branch from 905173c to b24d692 Compare September 13, 2024 16:01
@kvoli kvoli marked this pull request as ready for review September 13, 2024 16:01
@kvoli kvoli requested a review from a team as a code owner September 13, 2024 16:01
Copy link
Collaborator

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @kvoli)


pkg/kv/kvserver/kvflowcontrol/rac2/range_controller.go line 780 at r1 (raw file):

			}() {
			case replicate, probeRecentlyReplicate:
				rs.closeSendStream(ctx)

This change is already doing part of the cleanup from #130168 which eliminated snapshot connectedState. May as well do this now, instead of leaving cruft like changeToStateSnapshot{Locked} around.

@kvoli kvoli force-pushed the 240913.rac-send-stream-close branch from b24d692 to de91244 Compare September 13, 2024 17:07
Copy link
Collaborator Author

@kvoli kvoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TYFTR!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @kvoli and @sumeerbhola)


pkg/kv/kvserver/kvflowcontrol/rac2/range_controller.go line 780 at r1 (raw file):

Previously, sumeerbhola wrote…

This change is already doing part of the cleanup from #130168 which eliminated snapshot connectedState. May as well do this now, instead of leaving cruft like changeToStateSnapshot{Locked} around.

Yeah good idea, did the snapshot cleanup and delayed stream init now.

@kvoli kvoli force-pushed the 240913.rac-send-stream-close branch from de91244 to b62c94b Compare September 13, 2024 17:24
When a replica was removed from a range, or the range controller closed,
any tracked deductions did not return tokens to the respective stream
token counts.

Update `CloseRaftMuLocked` and `SetReplicasRaftMuLocked` to close any
replica send streams for replicas which are no longer part of the
replica set, closing all when the range controller closes.

Also immediately close the send stream when a replica transitions to
StateSnapshot.

Resolves: cockroachdb#130683
Release note: None
@kvoli kvoli force-pushed the 240913.rac-send-stream-close branch from b62c94b to 0cab9b5 Compare September 13, 2024 17:54
@kvoli
Copy link
Collaborator Author

kvoli commented Sep 13, 2024

=== RUN   TestLint/TestStaticCheck
    gen-lint_test.go:2079: 
        pkg/kv/kvserver/kvflowcontrol/rac2/range_controller.go:879:26: func connectedState.shouldWaitForElasticEvalTokens is unused (U1000)
    --- FAIL: TestLint/TestStaticCheck (1331.12s)

rm'd this function and pushed an amended the commit.

Copy link
Collaborator

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 3 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @kvoli)

@kvoli
Copy link
Collaborator Author

kvoli commented Sep 13, 2024

TYFTR!

bors r=sumeerbhola

@craig craig bot merged commit 0d1dca5 into cockroachdb:master Sep 13, 2024
22 of 23 checks passed
@kvoli kvoli linked an issue Sep 13, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants